Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (?) CodeEditorLink #31

Merged
merged 1 commit into from
Dec 28, 2022
Merged

Fix (?) CodeEditorLink #31

merged 1 commit into from
Dec 28, 2022

Conversation

abesto
Copy link
Contributor

@abesto abesto commented Dec 10, 2022

When attempting to use CodeEditorLink::with_editor, in my use-case the callback was never invoked, and with_editor would always return None. When printing the value of the link, I saw it did have a Some(...) in there, so I suspect something was up with the way Rc / borrows were used? I tried to understand exactly what's happening, but got lost in the Scope / Rc / RefCell soup. Another guess is that the Scope that got stored in the Rc<RefCell> on CodeEditorLink wasn't actually linked (heh) to the real scope, since it got a copy of the Scope.

I'm kinda experimenting my way through a fairly big design space, and I have some experience messing with Rc and friends, but I'm completely new to Yew, so I'll document my thought process so you can more easily tell me if / how I'm wrong.

See it in action:

Attempt 1

  • We only ever store CodeEditorModel inside an Rc<RefCell<Option<CodeEditorModel>>>. This smart pointer gets cloned to each new CodeEditorLink (instead of storing some reference to a Scope). This makes it easy to reason about what's going on - exactly the data we need is passed around, and it has a single place of storage.
  • I dropped the feature of allowing a CodeEditorLink to be passed in, for two reasons:
    • CodeEditorLink didn't expose a public constructor, and the .0 field is private, so there is in fact no way (AFAICT) to create a CodeEditorLink. This means the feature ... never worked? I think?
    • ctx.prop() gives us an &-reference, so to update the data in the CodeEditorLink passed in via props, we need internal mutability, which complicates things further. Dropping this allows the implementation to be simple, and user code can still store the CodeEditorLink instance received in the on_editor_created callback.

This however has a drawback: if user code wants to store the CodeEditorLink, then (at least in functional components) we kinda enforce double-rendering at startup: there's no clean way of saying "re-render if the link changes, except the first time when I store some special None value in the state". So:

Attempt 2

  • Added a public constructor
  • Re-added the link property
  • Wrapped the data in CodeEditorModel into a RefCell to enable internal mutability (no need for another wrapping Rc)

This is close, but there's still double-rendering in some circumstances. The core problem is: we don't have anything in CodeEditorLink for a stable comparison across rendering passes, only the reference to the model. Since that changes once the editor is created, the CodeEditorLink is also considered to have changed.

I messed around with a synthetic id, but didn't manage to get the behavior I wanted; ended up using TextModel instead to get the text out of the editor.

Again, this is not perfect; in particular, if on_editor_created depends on the CodeEditorLink, then we get double-rendering. But I figure it's an improvement still, so submitting the PR.

When attempting to use `CodeEditorLink::with_editor`, in my use-case the
callback would never be invoked, and `with_editor` would always return
`None`. When printing the value of the link, I saw it *did* have a
`Some(...)` in there, so I suspect something was up with the way `Rc` /
borrows were used? I tried to understand exactly what's
happening, but got lost in the `Scope` / `Rc` / `RefCell` soup. Another
guess is that the `Scope` that got stored in the `Rc<RefCell>>` on
`CodeEditorLink` wasn't actually linked (heh) to the *real* scope, since
that was a copy of the `Scope`.

I'm kinda experimenting my way through a fairly big design space, and I
have *some* experience messing with `Rc` and friends, but I'm completely
new to Yew, so I'll document my thought process so you can more easily
tell me if / how I'm wrong.

## Attempt 1

* We only ever store `CodeEditorModel` inside an
  `Rc<RefCell<Option<CodeEditorModel>>>`. This smart pointer gets cloned
  to each new `CodeEditorLink` (instead of storing some reference to a
  `Scope`). This makes it easy to reason about
  what's going on - exactly the data we need is passed around, and it
  has a single place of storage.
* I dropped the feature of allowing a `CodeEditorLink` to be passed in,
  for two reasons:
  * `CodeEditorLink` didn't expose a public constructor, and the `.0`
    field is private, so there is in fact no way (AFAICT) to create a
    `CodeEditorLink`. This means the feature ... never worked? I think?
  * `ctx.prop()` gives us an &-reference, so to update the data in the
    `CodeEditorLink` passed in via props, we need internal mutability,
    which complicates things further. Dropping this allows the
    implementation to be simple, and user code can still store the
    `CodeEditorLink` instance received in the `on_editor_created`
    callback.

This however has a drawback: if user code wants to store the
`CodeEditorLink`, then (at least in functional components) we kinda
enforce double-rendering at startup: there's no clean way of saying
"re-render if the link changes, except the first time when I store some
special `None` value in the state". So:

## Attempt 2

* Added a public constructor
* Re-added the `link` property
* Wrapped the data in `CodeEditorModel` into a `RefCell` to enable
  internal mutability (no need for another wrapping `Rc`)

This is close, but there's *still* double-rendering in some
circumstances. The core problem is: we don't have anything in
`CodeEditorLink` for a stable comparison across rendering passes,
*only* the reference to the model. Since that changes once the editor
is created, the `CodeEditorLink` is also considered to have changed.

I messed around with a synthetic id, but didn't manage to get the
behavior I wanted; ended up using `TextModel` instead to get the text
out of the editor.

Again, this is not perfect; in particular, if `on_editor_created`
depends on the `CodeEditorLink`, then we get double-rendering. But I
figure it's an improvement still, so submitting the PR.
@siku2
Copy link
Owner

siku2 commented Dec 28, 2022

I feel like this can be simplified a lot now, but I don't have the time to do that currently, so I really appreciate you making it work again.

@siku2 siku2 merged commit cac3ea1 into siku2:master Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants